ENH: __verify_trigger method and tests implementation#956
Open
juliomachad0 wants to merge 4 commits intoenh/eventsfrom
Open
ENH: __verify_trigger method and tests implementation#956juliomachad0 wants to merge 4 commits intoenh/eventsfrom
juliomachad0 wants to merge 4 commits intoenh/eventsfrom
Conversation
…out_of_rail_trigger to pass in __verify_trigger Co-authored-by: Copilot <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## enh/events #956 +/- ##
=============================================
Coverage ? 81.16%
=============================================
Files ? 108
Lines ? 13920
Branches ? 0
=============================================
Hits ? 11298
Misses ? 2622
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…on of the verify action method and adaptation of the out of rail trigger and action functions in flight class in order to pass in the new verifications Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements signature/type verification for Event trigger/action callables in rocketpy/simulation, adds initial unit tests for trigger verification, and adapts Flight’s out-of-rail event trigger/action to comply with the new trigger interface.
Changes:
- Added
Event.__verify_trigger/Event.__verify_actioncallable inspection (signature + return annotation checks). - Updated Flight’s out-of-rail trigger/action wiring to pass event data via keyword arguments.
- Added unit tests validating trigger signature/annotation validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
rocketpy/simulation/events.py |
Adds trigger/action verification logic using inspect.signature and type hints. |
rocketpy/simulation/flight.py |
Updates out-of-rail trigger to accept **kwargs and action invocation to use keyword args. |
tests/unit/simulation/test_events.py |
Adds tests for trigger verification error cases and acceptance. |
Comment on lines
+166
to
+176
| # verify if the return type annotation is None or dict | ||
| # Since is not possible to know for sure if the user is actually returning None or a dict, | ||
| # we enforce None or dict annotation to motivate users to actually return None or dict. | ||
| return_annotation = get_type_hints(action).get("return", None) | ||
| if return_annotation is not None and return_annotation is not ( | ||
| type(None) or dict | ||
| ): | ||
| raise ValueError( | ||
| f"The Action function of the {self.name} event must return None or a dictionary and must be annotated with '-> None' or '-> dict' for type checking.\n" | ||
| f"def {action.__name__}(**kwargs) -> None or dict:" | ||
| ) |
Comment on lines
+1051
to
+1070
| @@ -1062,6 +1065,9 @@ def __handle_out_of_rail_event(self, phase, phase_index, node_index): | |||
| bool | |||
| True to indicate the simulation should break. | |||
| """ | |||
| phase = kwargs.get("phase") | |||
| phase_index = kwargs.get("phase_index") | |||
| node_index = kwargs.get("node_index") | |||
Comment on lines
+6
to
+14
| def test_verify_trigger_accepts_only_kwargs(): | ||
| def trigger(**kwargs) -> bool: | ||
| return True | ||
|
|
||
| def action(**kwargs): | ||
| return None | ||
|
|
||
| event = Event(trigger=trigger, action=action, name="test") | ||
| assert event.trigger is trigger |
Comment on lines
+125
to
+127
| # verify if the trigger function accepts only **kwargs arguments | ||
| s = inspect.signature(trigger) | ||
| if any(p.kind != inspect.Parameter.VAR_KEYWORD for p in s.parameters.values()): |
…mplementation of new tests, mostly for verify actions method.
Member
Author
|
@MateusStano I think is ready to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCurrent behavior
Verify triggers function not implemented yet.
# TODO: check_trigger does note receive enough arguments to substitute parachute events
# TODO1: implement inspection of trigger function to verify:
# 1. It accepts **kwargs (accepts arbitrary keyword arguments)
# 2. Return type annotation is bool or can be tested to return bool
# 3. Consider allowing signature to be flexible (accepts **kwargs)
# to accommodate user-defined custom event_context keys
# TODO 2: implement inspection of action function to verify:
# 1. It accepts **kwargs (accepts arbitrary keyword arguments)
# 2. It can optionally return None or a dict with any of these keys:
# - Any custom keys to update event_context (dict type)
# 3. Raise ValueError if signature doesn't match expectations
New behavior
The verify_triggers method was implemented, trying to follow the instructions presented in the TODO. Also, an adaptation of the out_of_rail_trigger method in order to pass in the tests (following the defined standards)
Breaking change